Rename variables to remove references to 'global' global #12931

flarnie · 2018-05-29T23:35:02Z

what is the change?:
In a recent PR we were referencing some global variables and storing
local references to them.

To make things more natural, we co-opted the original name of the global
for our local reference. To make this work with Flow, we get the
original reference from 'window.requestAnimationFrame' and assign it to
'const requestAnimationFrame'.

Sometimes React is used in an environment where 'window' is not defined

in that case we need to use something else, or hide the 'window'
reference somewhere.

We opted to use 'global' thinking that Babel transforms would fill that
in with the proper thing.

But for some of our fixtures we are not doing that transform on the
bundle.

why make this change?:
I want to unbreak this on master and then investigate more about what we
should do to fix this.

Would like to land this quick fix but ultimately would like to avoid using the confusing 'localRequestAnimationFrame' naming.

test plan:
run yarn build and open the fixtures.

issue:

fixes #12930

#12930

**what is the change?:** In a recent PR we were referencing some global variables and storing local references to them. To make things more natural, we co-opted the original name of the global for our local reference. To make this work with Flow, we get the original reference from 'window.requestAnimationFrame' and assign it to 'const requestAnimationFrame'. Sometimes React is used in an environment where 'window' is not defined - in that case we need to use something else, or hide the 'window' reference somewhere. We opted to use 'global' thinking that Babel transforms would fill that in with the proper thing. But for some of our fixtures we are not doing that transform on the bundle. **why make this change?:** I want to unbreak this on master and then investigate more about what we should do to fix this. **test plan:** run `yarn build` and open the fixtures. **issue:** facebook#12930

gaearon · 2018-05-30T00:37:59Z

If it fails for fixtures I think it means it would fail after the release.

I don’t recall us using any transform that would turn global into a polyfill. So it’s probably just global in the output (which I would expect to break in any browser). JSDOM tests wouldn’t catch this so that’s why we missed it.

I’m not sure why ESLint checks on UMD bundles didn’t catch this. We have a whitelist if allowed globals. It’s worth following up to look at that. I guess maybe node environment is whitelisted there for the sake of UMD wrapper itself.

flarnie · 2018-05-30T00:54:32Z

Thanks. I think there is something I'm missing - will look into it, but I was also suprised the build tests didn't catch this.

sompylasar · 2018-06-08T07:20:11Z

@flarnie @gaearon This change seems to have broken the server-side rendering: I'm attempting to yarn start the fixtures/ssr to test #12063 and getting the following error from the server-side when I load the localhost:3000 page:

ReferenceError: requestAnimationFrame is not defined
    at /__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:5232:34
    at Object.<anonymous> (/__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:17632:5)
    at Module._compile (module.js:624:30)
    at Module._extensions..js (module.js:635:10)
    at Object.require.extensions.(anonymous function) [as .js] (/__CENSORED__/react/fixtures/ssr/node_modules/babel-register/lib/node.js:152:7)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Module.require (module.js:568:17)
    at require (internal/module.js:11:18)

It points to this line:

// We capture a local reference to any global, in case it gets polyfilled after
// this module is initially evaluated.
// We want to be using a consistent implementation.
const localRequestAnimationFrame = requestAnimationFrame;

gaearon · 2018-06-08T12:29:01Z

@sompylasar

Can you please send a fix? I imagine we’d want to only do this in a DOM environment. There should be an environment check later in that file.

sompylasar · 2018-06-08T14:47:19Z

@gaearon Something like:

const localRequestAnimationFrame = (
ExecutionEnvironment.canUseDOM &&
    typeof requestAnimationFrame === 'function'
    ? requestAnimationFrame
    : null
);

or something else? What do you want localRequestAnimationFrame to be if requestAnimationFrame is missing or invalid?

gaearon · 2018-06-08T15:07:10Z

Let's just export whatever it is equal to without checks. We check at the call site, and this module is temporary so it would be nice to keep it simple.

const localRequestAnimationFrame =
  typeof requestAnimationFrame === 'function'
    ? requestAnimationFrame
    : undefined;

sompylasar · 2018-06-08T15:10:22Z

Ok, undefined it will be then.

…facebook#13000) The facebook#12931 ( facebook@79a740c ) broke the server-side rendering: in the `fixtures/ssr` the following error appeared from the server-side when `localhost:3000` is requested: ``` ReferenceError: requestAnimationFrame is not defined at /__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:5232:34 at Object.<anonymous> (/__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:17632:5) at Module._compile (module.js:624:30) at Module._extensions..js (module.js:635:10) at Object.require.extensions.(anonymous function) [as .js] (/__CENSORED__/react/fixtures/ssr/node_modules/babel-register/lib/node.js:152:7) at Module.load (module.js:545:32) at tryModuleLoad (module.js:508:12) at Function.Module._load (module.js:500:3) at Module.require (module.js:568:17) at require (internal/module.js:11:18) ``` The exception pointed to this line: ```js // We capture a local reference to any global, in case it gets polyfilled after // this module is initially evaluated. // We want to be using a consistent implementation. const localRequestAnimationFrame = requestAnimationFrame; ``` **Test plan** 1. In `react` repo root, `yarn && yarn build`. 2. In `fixtures/ssr`, `yarn && yarn start`, 3. In browser, go to `http://localhost:3000`. 4. Observe the fixture page, not the exception message.

sompylasar · 2018-06-08T15:31:55Z

👉 #13000 fixed in #13001

@gaearon

…#13000) (#13001) * Fix react-dom ReferenceError requestAnimationFrame in non-browser env (#13000) The #12931 ( 79a740c ) broke the server-side rendering: in the `fixtures/ssr` the following error appeared from the server-side when `localhost:3000` is requested: ``` ReferenceError: requestAnimationFrame is not defined at /__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:5232:34 at Object.<anonymous> (/__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:17632:5) at Module._compile (module.js:624:30) at Module._extensions..js (module.js:635:10) at Object.require.extensions.(anonymous function) [as .js] (/__CENSORED__/react/fixtures/ssr/node_modules/babel-register/lib/node.js:152:7) at Module.load (module.js:545:32) at tryModuleLoad (module.js:508:12) at Function.Module._load (module.js:500:3) at Module.require (module.js:568:17) at require (internal/module.js:11:18) ``` The exception pointed to this line: ```js // We capture a local reference to any global, in case it gets polyfilled after // this module is initially evaluated. // We want to be using a consistent implementation. const localRequestAnimationFrame = requestAnimationFrame; ``` **Test plan** 1. In `react` repo root, `yarn && yarn build`. 2. In `fixtures/ssr`, `yarn && yarn start`, 3. In browser, go to `http://localhost:3000`. 4. Observe the fixture page, not the exception message. * Move the requestAnimationFrameForReact check and warning to callsites (#13000) According to the comment by @gaearon: #13001 (comment) * Use `invariant` instead of `throw new Error`, use the same message (#13000) According to the comment by @gaearon: #13001 (comment)

flarnie requested review from gaearon and acdlite May 29, 2018 23:35

facebook-github-bot added the CLA Signed label May 29, 2018

gaearon approved these changes May 30, 2018

View reviewed changes

flarnie merged commit 79a740c into facebook:master May 30, 2018

sompylasar mentioned this pull request Jun 8, 2018

DOM diff and component stack in SSR hydrate mismatch warnings (#10085) #12063

Closed

sompylasar mentioned this pull request Jun 8, 2018

react-dom throws "ReferenceError: requestAnimationFrame is not defined" in SSR environment since #12931 #13000

Closed

sompylasar mentioned this pull request Jun 8, 2018

Fix react-dom ReferenceError requestAnimationFrame in non-browser env (#13000) #13001

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename variables to remove references to 'global' global #12931

Rename variables to remove references to 'global' global #12931

flarnie commented May 29, 2018

gaearon commented May 30, 2018

flarnie commented May 30, 2018

sompylasar commented Jun 8, 2018 •

edited

Loading

gaearon commented Jun 8, 2018

sompylasar commented Jun 8, 2018

gaearon commented Jun 8, 2018

sompylasar commented Jun 8, 2018

sompylasar commented Jun 8, 2018

Rename variables to remove references to 'global' global #12931

Rename variables to remove references to 'global' global #12931

Conversation

flarnie commented May 29, 2018

gaearon commented May 30, 2018

flarnie commented May 30, 2018

sompylasar commented Jun 8, 2018 • edited Loading

gaearon commented Jun 8, 2018

sompylasar commented Jun 8, 2018

gaearon commented Jun 8, 2018

sompylasar commented Jun 8, 2018

sompylasar commented Jun 8, 2018

sompylasar commented Jun 8, 2018 •

edited

Loading